Skip to content

feat(embedding): declare a model's width without sending dimensions - #1857

Open
NullSense wants to merge 1 commit into
repowise-dev:mainfrom
NullSense:feat/declared-only-embedding-dims
Open

feat(embedding): declare a model's width without sending dimensions#1857
NullSense wants to merge 1 commit into
repowise-dev:mainfrom
NullSense:feat/declared-only-embedding-dims

Conversation

@NullSense

Copy link
Copy Markdown

Summary

  • OpenAIEmbedder can only override an embedding model's width by also sending that width to the API as the dimensions parameter. That's correct for a Matryoshka model, but it breaks a model of non-default width that isn't Matryoshka-capable: the API rejects the request outright (e.g. nvidia/Nemotron-3-Embed-1B, 2048-wide, served OpenAI-compatible, errors with "does not support Matryoshka embeddings; dimensions must be unset").
  • Adds declared_dimensions / REPOWISE_EMBEDDING_DECLARED_DIMS, a second override that sets the declared width without ever sending it. Precedence: dimensions= > REPOWISE_EMBEDDING_DIMS > declared_dimensions= > REPOWISE_EMBEDDING_DECLARED_DIMS > the _DIMS table > 1536. Only the first two are ever sent on the wire.
  • The width-mismatch error now names whichever of the three sources actually chose the number, instead of always pointing at _DIMS or REPOWISE_EMBEDDING_DIMS.

Related Issues

None.

Test Plan

  • uv run pytest tests/unit/test_persistence/test_openai_embedder.py -q — 52 passed
  • uv run pytest tests/unit/test_persistence/ tests/unit/test_providers/ tests/unit/server/mcp/test_embedder_resolution.py tests/unit/cli/test_embedder_key_resolution.py -q — 416 passed
  • uv run ruff check . (the actual CI lint gate; ruff format --check isn't run in CI, and this PR's files are clean under it anyway)
  • uv run repowise risk main..HEAD:
    Change risk for main..HEAD: touches files that have broken before · 52nd percentile of this repo's fix-bearing files
      feat(embedding): declare a model's width without sending dimensions
      +212 / -36 lines · 2 files · 2 dirs · 2 subsystems · entropy 0.98 · author exp 0
    
    File                                                              Lines   Prior fixes
    packages/core/src/repowise/core/providers/embedding/openai.py       142           1.0
    tests/unit/test_persistence/test_openai_embedder.py                 106           1.0
    
    Diff shape: Typical · 48th percentile of recent commits by size and spread
      About as risky as a typical commit in this repo.
    
    (impacted-tests / health --file need a completed repowise init against this clone; that indexing run did not finish in time and is omitted rather than faked.)

Why this shape, not a _DIMS table entry

The straightforward fix is hardcoding "nemotron-embed-1b": 2048 in _DIMS. That covers exactly one deployment; every other non-Matryoshka model of non-default width hits the identical wall. The existing dimensions= design already treats width as something the operator declares for whatever model they're running, not something the code enumerates by name — declared_dimensions extends that same idea to the one case it didn't cover.

Checklist

  • My code follows the project's code style
  • I have added tests for new functionality
  • All existing tests still pass
  • I have updated documentation if needed

Written with AI assistance; measurements and tests were run and verified locally.

@Ayush7614 Ayush7614 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified: pytest tests/unit/test_persistence/test_openai_embedder.py -q 52 passed. Correctly adds declared_dimensions/REPOWISE_EMBEDDING_DECLARED_DIMS as declare-only width (never sent as dimensions param) for non-Matryoshka models like Nemotron-3-Embed-1B that 400 on dimensions. Precedence dimensions > REPOWISE_EMBEDDING_DIMS > declared_dimensions > REPOWISE_EMBEDDING_DECLARED_DIMS > _DIMS > 1536 matches spec, error messages now name the actual source, and the strict-endpoint regression test proves the param is never sent. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants